[UUM-133527] - Improve floating point precision when Probuilderizing small objects#658
Open
[UUM-133527] - Improve floating point precision when Probuilderizing small objects#658
Conversation
…o closed vertices were getting removed because of lack of precision
| static class VectorHash | ||
| { | ||
| public const float FltCompareResolution = 1000f; | ||
| public const float FltCompareResolution = 1000000f; |
There was a problem hiding this comment.
Have you considered the impact of this new resolution on float precision limits and potential integer overflows?
Increasing FltCompareResolution to 1000000f introduces two major issues:
-
Integer Overflow: Conversions in
VectorHashand related structs (likeIntVec3) multiply vertex coordinates by this resolution and convert them usingSystem.Convert.ToInt32(). With a multiplier of 1,000,000, any coordinate with a magnitude greater than~2147.48will exceedint.MaxValue, triggering a runtimeOverflowExceptionand crashing the mesh generation process. -
Hash Stability: The purpose of this resolution is to mask floating-point imprecision so that spatially identical vertices hash to the same value. A 32-bit
floatonly provides ~7 significant decimal digits. A scale of1,000,000demands exactness up to$10^{-6}$ . For any coordinate$\ge 10$ , this exceeds standard float precision limits. Minor math inaccuracies will cause virtually identical vertices to resolve to different integers, which will break vertex welding, duplicate detection, and spatial sorting.
I highly recommend reverting this to 1000f (which provides a reliable
🤖 Helpful? 👍/👎
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## master #658 +/- ##
=======================================
Coverage 36.05% 36.06%
=======================================
Files 277 277
Lines 34909 34910 +1
=======================================
+ Hits 12588 12589 +1
Misses 22321 22321
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DO NOT FORGET TO INCLUDE A CHANGELOG ENTRY
Purpose of this PR
[Brief summary of the changes in this PR.]
Links
Jira: https://jira.unity3d.com/browse/UUM-133527
Comments to Reviewers
[List known issues, planned work, provide any extra context for your code.]